From 5f0ecdb6d0dea3d49d6e9e9b1915797cbc8077d3 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Fri, 24 Apr 2026 11:50:35 +0000 Subject: [PATCH 1/5] fix: replace authlib with requests to eliminate AuthlibDeprecationWarning Remove the authlib dependency which triggers a deprecation warning from its internal authlib.jose module on every import. Replace the OAuth2Session usage with a lightweight _OAuth2Client class that uses requests.Session directly for OAuth2 flows. Fixes #627 Co-Authored-By: Claude Opus 4.6 --- .../jumpstarter_cli_common/oidc.py | 75 ++++++++++++++++++- .../jumpstarter-cli-common/pyproject.toml | 2 +- python/uv.lock | 17 +---- 3 files changed, 76 insertions(+), 18 deletions(-) diff --git a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py index 08661c853..c97e10df1 100644 --- a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py +++ b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py @@ -1,24 +1,95 @@ import json import os +import secrets import ssl import time from dataclasses import dataclass from functools import wraps from typing import ClassVar +from urllib.parse import parse_qs, urlencode, urlparse import aiohttp import certifi import click +import requests from aiohttp import web from anyio import create_memory_object_stream from anyio.to_thread import run_sync -from authlib.integrations.requests_client import OAuth2Session from joserfc.jws import extract_compact from yarl import URL from jumpstarter.config.env import JMP_OIDC_CALLBACK_PORT +class _OAuth2Client: + """Lightweight OAuth2 client using requests.Session. + + Replaces authlib.integrations.requests_client.OAuth2Session to avoid + the AuthlibDeprecationWarning triggered by authlib's internal + authlib.jose imports (see https://github.com/jumpstarter-dev/jumpstarter/issues/627). + """ + + def __init__(self, client_id: str, scope: list[str], redirect_uri: str | None = None): + self.client_id = client_id + self.scope = scope + self.redirect_uri = redirect_uri + self._session = requests.Session() + + @property + def verify(self): + return self._session.verify + + @verify.setter + def verify(self, value): + self._session.verify = value + + def create_authorization_url(self, url: str, **kwargs) -> tuple[str, str]: + """Build an authorization URL with a generated state parameter.""" + state = secrets.token_urlsafe(32) + params = { + "response_type": "code", + "client_id": self.client_id, + "scope": " ".join(self.scope), + "state": state, + } + if self.redirect_uri: + params["redirect_uri"] = self.redirect_uri + params.update(kwargs) + separator = "&" if "?" in url else "?" + return f"{url}{separator}{urlencode(params)}", state + + def fetch_token(self, url: str, **kwargs) -> dict: + """Exchange credentials for an OAuth2 token via HTTP POST.""" + data = {"client_id": self.client_id} + + # Handle authorization_response: extract the code from the callback URL + authorization_response = kwargs.pop("authorization_response", None) + if authorization_response: + parsed = urlparse(authorization_response) + qs = parse_qs(parsed.query) + code = qs.get("code", [None])[0] + if code: + data["code"] = code + if self.redirect_uri: + data["redirect_uri"] = self.redirect_uri + data.setdefault("grant_type", "authorization_code") + + # Merge remaining kwargs into the POST body + data.update(kwargs) + + # Ensure scope is a space-separated string + if "scope" not in data: + data["scope"] = " ".join(self.scope) + + response = self._session.post( + url, + data=data, + headers={"Accept": "application/json"}, + ) + response.raise_for_status() + return response.json() + + def _get_ssl_context() -> ssl.SSLContext: """Create an SSL context that respects SSL_CERT_FILE environment variable.""" ssl_ctx = ssl.create_default_context() @@ -77,7 +148,7 @@ def _scopes(self) -> list[str]: return list(self.scope) def client(self, **kwargs): - session = OAuth2Session(client_id=self.client_id, scope=self._scopes(), **kwargs) + session = _OAuth2Client(client_id=self.client_id, scope=self._scopes(), **kwargs) session.verify = False if self.insecure_tls else (os.environ.get("SSL_CERT_FILE") or certifi.where()) return session diff --git a/python/packages/jumpstarter-cli-common/pyproject.toml b/python/packages/jumpstarter-cli-common/pyproject.toml index e892545fa..701a52858 100644 --- a/python/packages/jumpstarter-cli-common/pyproject.toml +++ b/python/packages/jumpstarter-cli-common/pyproject.toml @@ -10,7 +10,7 @@ dependencies = [ "jumpstarter", "pydantic>=2.8.2", "click>=8.1.7.2", - "authlib>=1.4.1", + "requests>=2.32.0", "truststore>=0.10.1", "joserfc>=1.0.3", "yarl>=1.18.3", diff --git a/python/uv.lock b/python/uv.lock index 23f95c4eb..5dd853b9e 100644 --- a/python/uv.lock +++ b/python/uv.lock @@ -364,19 +364,6 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/77/06/bb80f5f86020c4551da315d78b3ab75e8228f89f0162f2c3a819e407941a/attrs-25.3.0-py3-none-any.whl", hash = "sha256:427318ce031701fea540783410126f03899a97ffc6f61596ad581ac2e40e3bc3", size = 63815, upload-time = "2025-03-13T11:10:21.14Z" }, ] -[[package]] -name = "authlib" -version = "1.6.0" -source = { registry = "https://pypi.org/simple" } -dependencies = [ - { name = "cryptography", version = "44.0.3", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" }, - { name = "cryptography", version = "45.0.4", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.12'" }, -] -sdist = { url = "https://files.pythonhosted.org/packages/a2/9d/b1e08d36899c12c8b894a44a5583ee157789f26fc4b176f8e4b6217b56e1/authlib-1.6.0.tar.gz", hash = "sha256:4367d32031b7af175ad3a323d571dc7257b7099d55978087ceae4a0d88cd3210", size = 158371, upload-time = "2025-05-23T00:21:45.011Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/84/29/587c189bbab1ccc8c86a03a5d0e13873df916380ef1be461ebe6acebf48d/authlib-1.6.0-py2.py3-none-any.whl", hash = "sha256:91685589498f79e8655e8a8947431ad6288831d643f11c55c2143ffcc738048d", size = 239981, upload-time = "2025-05-23T00:21:43.075Z" }, -] - [[package]] name = "babel" version = "2.17.0" @@ -2154,11 +2141,11 @@ dev = [ name = "jumpstarter-cli-common" source = { editable = "packages/jumpstarter-cli-common" } dependencies = [ - { name = "authlib" }, { name = "click" }, { name = "joserfc" }, { name = "jumpstarter" }, { name = "pydantic" }, + { name = "requests" }, { name = "rich" }, { name = "truststore" }, { name = "yarl" }, @@ -2174,11 +2161,11 @@ dev = [ [package.metadata] requires-dist = [ - { name = "authlib", specifier = ">=1.4.1" }, { name = "click", specifier = ">=8.1.7.2" }, { name = "joserfc", specifier = ">=1.0.3" }, { name = "jumpstarter", editable = "packages/jumpstarter" }, { name = "pydantic", specifier = ">=2.8.2" }, + { name = "requests", specifier = ">=2.32.0" }, { name = "rich", specifier = ">=14.0.0" }, { name = "truststore", specifier = ">=0.10.1" }, { name = "yarl", specifier = ">=1.18.3" }, From e5e6b64450bdc6dec34f0d7c07d20969cf09909a Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Fri, 24 Apr 2026 12:35:38 +0000 Subject: [PATCH 2/5] test: add unit tests for _OAuth2Client class to fix diff-cover check Add tests covering __init__, verify property, create_authorization_url, fetch_token, and Config.client to bring coverage on changed lines from 26.2% to 100%, meeting the 80% threshold required by diff-cover CI. Co-Authored-By: Claude Opus 4.6 --- .../jumpstarter_cli_common/test_oidc.py | 228 ++++++++++++++++++ 1 file changed, 228 insertions(+) create mode 100644 python/packages/jumpstarter-cli-common/jumpstarter_cli_common/test_oidc.py diff --git a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/test_oidc.py b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/test_oidc.py new file mode 100644 index 000000000..54a9883d0 --- /dev/null +++ b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/test_oidc.py @@ -0,0 +1,228 @@ +from unittest.mock import MagicMock, patch +from urllib.parse import parse_qs, urlparse + +import pytest +import requests + +from jumpstarter_cli_common.oidc import Config, _OAuth2Client + + +class TestOAuth2ClientInit: + def test_init_sets_attributes(self): + client = _OAuth2Client(client_id="my-client", scope=["openid", "profile"]) + assert client.client_id == "my-client" + assert client.scope == ["openid", "profile"] + assert client.redirect_uri is None + assert isinstance(client._session, requests.Session) + + def test_init_with_redirect_uri(self): + client = _OAuth2Client( + client_id="my-client", + scope=["openid"], + redirect_uri="http://localhost:8080/callback", + ) + assert client.redirect_uri == "http://localhost:8080/callback" + + +class TestOAuth2ClientVerifyProperty: + def test_verify_getter_returns_session_verify(self): + client = _OAuth2Client(client_id="c", scope=[]) + # requests.Session defaults verify to True + assert client.verify is True + + def test_verify_setter_updates_session_verify(self): + client = _OAuth2Client(client_id="c", scope=[]) + client.verify = False + assert client.verify is False + assert client._session.verify is False + + def test_verify_setter_with_cert_path(self): + client = _OAuth2Client(client_id="c", scope=[]) + client.verify = "/path/to/ca-bundle.crt" + assert client.verify == "/path/to/ca-bundle.crt" + assert client._session.verify == "/path/to/ca-bundle.crt" + + +class TestOAuth2ClientCreateAuthorizationUrl: + def test_basic_url_construction(self): + client = _OAuth2Client(client_id="test-client", scope=["openid", "profile"]) + url, state = client.create_authorization_url("https://auth.example.com/authorize") + + parsed = urlparse(url) + params = parse_qs(parsed.query) + + assert parsed.scheme == "https" + assert parsed.netloc == "auth.example.com" + assert parsed.path == "/authorize" + assert params["response_type"] == ["code"] + assert params["client_id"] == ["test-client"] + assert params["scope"] == ["openid profile"] + assert params["state"] == [state] + assert len(state) > 0 + + def test_includes_redirect_uri_when_set(self): + client = _OAuth2Client( + client_id="test-client", + scope=["openid"], + redirect_uri="http://localhost:9999/callback", + ) + url, _state = client.create_authorization_url("https://auth.example.com/authorize") + params = parse_qs(urlparse(url).query) + assert params["redirect_uri"] == ["http://localhost:9999/callback"] + + def test_no_redirect_uri_when_not_set(self): + client = _OAuth2Client(client_id="test-client", scope=["openid"]) + url, _state = client.create_authorization_url("https://auth.example.com/authorize") + params = parse_qs(urlparse(url).query) + assert "redirect_uri" not in params + + def test_extra_kwargs_included(self): + client = _OAuth2Client(client_id="test-client", scope=["openid"]) + url, _state = client.create_authorization_url( + "https://auth.example.com/authorize", + prompt="consent", + nonce="abc123", + ) + params = parse_qs(urlparse(url).query) + assert params["prompt"] == ["consent"] + assert params["nonce"] == ["abc123"] + + def test_url_with_existing_query_params(self): + client = _OAuth2Client(client_id="test-client", scope=["openid"]) + url, _state = client.create_authorization_url("https://auth.example.com/authorize?foo=bar") + # Should use '&' separator since URL already has '?' + assert "authorize?foo=bar&" in url + + def test_state_is_unique(self): + client = _OAuth2Client(client_id="test-client", scope=["openid"]) + _, state1 = client.create_authorization_url("https://auth.example.com/authorize") + _, state2 = client.create_authorization_url("https://auth.example.com/authorize") + assert state1 != state2 + + +class TestOAuth2ClientFetchToken: + def _mock_response(self, json_data, status_code=200): + mock_resp = MagicMock() + mock_resp.json.return_value = json_data + mock_resp.raise_for_status.return_value = None + mock_resp.status_code = status_code + return mock_resp + + def test_fetch_token_with_grant_type(self): + token_data = {"access_token": "tok123", "token_type": "Bearer"} + client = _OAuth2Client(client_id="my-client", scope=["openid", "profile"]) + + with patch.object(client._session, "post", return_value=self._mock_response(token_data)) as mock_post: + result = client.fetch_token( + "https://auth.example.com/token", + grant_type="password", + username="user", + password="pass", + ) + + assert result == token_data + call_kwargs = mock_post.call_args + post_data = call_kwargs.kwargs["data"] + assert post_data["client_id"] == "my-client" + assert post_data["grant_type"] == "password" + assert post_data["username"] == "user" + assert post_data["password"] == "pass" + assert post_data["scope"] == "openid profile" + assert call_kwargs.kwargs["headers"] == {"Accept": "application/json"} + + def test_fetch_token_with_authorization_response(self): + token_data = {"access_token": "tok456", "token_type": "Bearer"} + client = _OAuth2Client( + client_id="my-client", + scope=["openid"], + redirect_uri="http://localhost:8080/callback", + ) + + callback_url = "http://localhost:8080/callback?code=authcode123&state=xyz" + + with patch.object(client._session, "post", return_value=self._mock_response(token_data)) as mock_post: + result = client.fetch_token( + "https://auth.example.com/token", + authorization_response=callback_url, + ) + + assert result == token_data + post_data = mock_post.call_args.kwargs["data"] + assert post_data["code"] == "authcode123" + assert post_data["redirect_uri"] == "http://localhost:8080/callback" + assert post_data["grant_type"] == "authorization_code" + + def test_fetch_token_authorization_response_no_code(self): + token_data = {"access_token": "tok789"} + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + + callback_url = "http://localhost:8080/callback?state=xyz" + + with patch.object(client._session, "post", return_value=self._mock_response(token_data)) as mock_post: + result = client.fetch_token( + "https://auth.example.com/token", + authorization_response=callback_url, + ) + + assert result == token_data + post_data = mock_post.call_args.kwargs["data"] + assert "code" not in post_data + + def test_fetch_token_authorization_response_without_redirect_uri(self): + token_data = {"access_token": "tok000"} + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + # redirect_uri is None + + callback_url = "http://localhost:8080/callback?code=abc" + + with patch.object(client._session, "post", return_value=self._mock_response(token_data)) as mock_post: + client.fetch_token( + "https://auth.example.com/token", + authorization_response=callback_url, + ) + + post_data = mock_post.call_args.kwargs["data"] + assert "redirect_uri" not in post_data + + def test_fetch_token_scope_provided_in_kwargs(self): + token_data = {"access_token": "tok999"} + client = _OAuth2Client(client_id="my-client", scope=["openid", "profile"]) + + with patch.object(client._session, "post", return_value=self._mock_response(token_data)) as mock_post: + client.fetch_token( + "https://auth.example.com/token", + grant_type="client_credentials", + scope="custom_scope", + ) + + post_data = mock_post.call_args.kwargs["data"] + # When scope is provided in kwargs, it should not be overridden + assert post_data["scope"] == "custom_scope" + + def test_fetch_token_raises_on_http_error(self): + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + mock_resp = MagicMock() + mock_resp.raise_for_status.side_effect = requests.HTTPError("401 Unauthorized") + + with patch.object(client._session, "post", return_value=mock_resp): + with pytest.raises(requests.HTTPError): + client.fetch_token("https://auth.example.com/token", grant_type="password") + + +class TestConfigClient: + def test_client_returns_oauth2_client(self): + config = Config(issuer="https://issuer.example.com", client_id="test-client") + session = config.client() + assert isinstance(session, _OAuth2Client) + assert session.client_id == "test-client" + assert session.scope == ["openid", "profile"] + + def test_client_with_insecure_tls(self): + config = Config(issuer="https://issuer.example.com", client_id="test-client", insecure_tls=True) + session = config.client() + assert session.verify is False + + def test_client_passes_kwargs(self): + config = Config(issuer="https://issuer.example.com", client_id="test-client") + session = config.client(redirect_uri="http://localhost:9999/callback") + assert session.redirect_uri == "http://localhost:9999/callback" From 4f6a7240c832bfdcd9e9d8b86ab4a837c9216eef Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Thu, 28 May 2026 11:34:14 +0000 Subject: [PATCH 3/5] fix: address review feedback on OAuth2 client implementation - Add CSRF protection: validate state parameter in fetch_token against expected state stored during create_authorization_url - Handle OAuth2 error responses in authorization callback URLs - Add close() method to _OAuth2Client; close sessions in all grant methods - Add type annotations to verify property (bool | str) - Add explicit Content-Type header for token requests - Handle non-JSON token endpoint responses with descriptive error - Use dict[str, Any] return type for fetch_token - Shorten docstring to describe contract, not history - Remove redundant inline comments - Rename test file to oauth2_client_test.py (project convention) - Remove duplicate insecure_tls test (already in oidc_test.py) - Add tests for Config grant methods (token_exchange, refresh, password) - Add state validation and error response tests - Add deprecation warning regression test Co-Authored-By: Claude Opus 4.6 --- .../{test_oidc.py => oauth2_client_test.py} | 211 ++++++++++++++++-- .../jumpstarter_cli_common/oidc.py | 125 +++++++---- 2 files changed, 274 insertions(+), 62 deletions(-) rename python/packages/jumpstarter-cli-common/jumpstarter_cli_common/{test_oidc.py => oauth2_client_test.py} (50%) diff --git a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/test_oidc.py b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oauth2_client_test.py similarity index 50% rename from python/packages/jumpstarter-cli-common/jumpstarter_cli_common/test_oidc.py rename to python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oauth2_client_test.py index 54a9883d0..058fe28f3 100644 --- a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/test_oidc.py +++ b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oauth2_client_test.py @@ -1,10 +1,16 @@ -from unittest.mock import MagicMock, patch +import warnings +from unittest.mock import AsyncMock, MagicMock, patch from urllib.parse import parse_qs, urlparse import pytest import requests -from jumpstarter_cli_common.oidc import Config, _OAuth2Client +from jumpstarter_cli_common.oidc import ( + Config, + OAuthError, + OAuthStateMismatchError, + _OAuth2Client, +) class TestOAuth2ClientInit: @@ -13,6 +19,7 @@ def test_init_sets_attributes(self): assert client.client_id == "my-client" assert client.scope == ["openid", "profile"] assert client.redirect_uri is None + assert client._expected_state is None assert isinstance(client._session, requests.Session) def test_init_with_redirect_uri(self): @@ -27,7 +34,6 @@ def test_init_with_redirect_uri(self): class TestOAuth2ClientVerifyProperty: def test_verify_getter_returns_session_verify(self): client = _OAuth2Client(client_id="c", scope=[]) - # requests.Session defaults verify to True assert client.verify is True def test_verify_setter_updates_session_verify(self): @@ -43,6 +49,14 @@ def test_verify_setter_with_cert_path(self): assert client._session.verify == "/path/to/ca-bundle.crt" +class TestOAuth2ClientClose: + def test_close_calls_session_close(self): + client = _OAuth2Client(client_id="c", scope=[]) + with patch.object(client._session, "close") as mock_close: + client.close() + mock_close.assert_called_once() + + class TestOAuth2ClientCreateAuthorizationUrl: def test_basic_url_construction(self): client = _OAuth2Client(client_id="test-client", scope=["openid", "profile"]) @@ -60,6 +74,11 @@ def test_basic_url_construction(self): assert params["state"] == [state] assert len(state) > 0 + def test_stores_expected_state(self): + client = _OAuth2Client(client_id="test-client", scope=["openid"]) + _, state = client.create_authorization_url("https://auth.example.com/authorize") + assert client._expected_state == state + def test_includes_redirect_uri_when_set(self): client = _OAuth2Client( client_id="test-client", @@ -90,7 +109,6 @@ def test_extra_kwargs_included(self): def test_url_with_existing_query_params(self): client = _OAuth2Client(client_id="test-client", scope=["openid"]) url, _state = client.create_authorization_url("https://auth.example.com/authorize?foo=bar") - # Should use '&' separator since URL already has '?' assert "authorize?foo=bar&" in url def test_state_is_unique(self): @@ -128,7 +146,17 @@ def test_fetch_token_with_grant_type(self): assert post_data["username"] == "user" assert post_data["password"] == "pass" assert post_data["scope"] == "openid profile" - assert call_kwargs.kwargs["headers"] == {"Accept": "application/json"} + + def test_fetch_token_sends_correct_headers(self): + token_data = {"access_token": "tok123"} + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + + with patch.object(client._session, "post", return_value=self._mock_response(token_data)) as mock_post: + client.fetch_token("https://auth.example.com/token", grant_type="password") + + headers = mock_post.call_args.kwargs["headers"] + assert headers["Accept"] == "application/json" + assert headers["Content-Type"] == "application/x-www-form-urlencoded" def test_fetch_token_with_authorization_response(self): token_data = {"access_token": "tok456", "token_type": "Bearer"} @@ -137,6 +165,7 @@ def test_fetch_token_with_authorization_response(self): scope=["openid"], redirect_uri="http://localhost:8080/callback", ) + client._expected_state = "xyz" callback_url = "http://localhost:8080/callback?code=authcode123&state=xyz" @@ -152,26 +181,69 @@ def test_fetch_token_with_authorization_response(self): assert post_data["redirect_uri"] == "http://localhost:8080/callback" assert post_data["grant_type"] == "authorization_code" - def test_fetch_token_authorization_response_no_code(self): + def test_fetch_token_validates_state(self): + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + client._expected_state = "expected-state" + + callback_url = "http://localhost:8080/callback?code=abc&state=wrong-state" + + with pytest.raises(OAuthStateMismatchError, match="state mismatch"): + client.fetch_token( + "https://auth.example.com/token", + authorization_response=callback_url, + ) + + def test_fetch_token_validates_state_missing_in_response(self): + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + client._expected_state = "expected-state" + + callback_url = "http://localhost:8080/callback?code=abc" + + with pytest.raises(OAuthStateMismatchError, match="state mismatch"): + client.fetch_token( + "https://auth.example.com/token", + authorization_response=callback_url, + ) + + def test_fetch_token_skips_state_validation_when_no_expected_state(self): token_data = {"access_token": "tok789"} client = _OAuth2Client(client_id="my-client", scope=["openid"]) + # _expected_state is None (no create_authorization_url was called) - callback_url = "http://localhost:8080/callback?state=xyz" + callback_url = "http://localhost:8080/callback?code=abc&state=any" - with patch.object(client._session, "post", return_value=self._mock_response(token_data)) as mock_post: + with patch.object(client._session, "post", return_value=self._mock_response(token_data)): result = client.fetch_token( "https://auth.example.com/token", authorization_response=callback_url, ) - assert result == token_data - post_data = mock_post.call_args.kwargs["data"] - assert "code" not in post_data + + def test_fetch_token_raises_on_oauth_error_response(self): + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + + callback_url = "http://localhost:8080/callback?error=access_denied&error_description=User+denied+access" + + with pytest.raises(OAuthError, match="access_denied.*User denied access"): + client.fetch_token( + "https://auth.example.com/token", + authorization_response=callback_url, + ) + + def test_fetch_token_raises_on_oauth_error_without_description(self): + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + + callback_url = "http://localhost:8080/callback?error=server_error" + + with pytest.raises(OAuthError, match="server_error"): + client.fetch_token( + "https://auth.example.com/token", + authorization_response=callback_url, + ) def test_fetch_token_authorization_response_without_redirect_uri(self): token_data = {"access_token": "tok000"} client = _OAuth2Client(client_id="my-client", scope=["openid"]) - # redirect_uri is None callback_url = "http://localhost:8080/callback?code=abc" @@ -196,7 +268,6 @@ def test_fetch_token_scope_provided_in_kwargs(self): ) post_data = mock_post.call_args.kwargs["data"] - # When scope is provided in kwargs, it should not be overridden assert post_data["scope"] == "custom_scope" def test_fetch_token_raises_on_http_error(self): @@ -208,6 +279,17 @@ def test_fetch_token_raises_on_http_error(self): with pytest.raises(requests.HTTPError): client.fetch_token("https://auth.example.com/token", grant_type="password") + def test_fetch_token_raises_on_non_json_response(self): + client = _OAuth2Client(client_id="my-client", scope=["openid"]) + mock_resp = MagicMock() + mock_resp.raise_for_status.return_value = None + mock_resp.status_code = 200 + mock_resp.json.side_effect = ValueError("No JSON") + + with patch.object(client._session, "post", return_value=mock_resp): + with pytest.raises(OAuthError, match="non-JSON response"): + client.fetch_token("https://auth.example.com/token", grant_type="password") + class TestConfigClient: def test_client_returns_oauth2_client(self): @@ -217,12 +299,105 @@ def test_client_returns_oauth2_client(self): assert session.client_id == "test-client" assert session.scope == ["openid", "profile"] - def test_client_with_insecure_tls(self): - config = Config(issuer="https://issuer.example.com", client_id="test-client", insecure_tls=True) - session = config.client() - assert session.verify is False - def test_client_passes_kwargs(self): config = Config(issuer="https://issuer.example.com", client_id="test-client") session = config.client(redirect_uri="http://localhost:9999/callback") assert session.redirect_uri == "http://localhost:9999/callback" + + +class TestConfigGrantMethods: + """Tests for Config grant methods that use _OAuth2Client.fetch_token.""" + + @pytest.fixture + def config(self): + return Config(issuer="https://issuer.example.com", client_id="test-client") + + @pytest.fixture + def mock_configuration(self): + return { + "token_endpoint": "https://issuer.example.com/token", + "authorization_endpoint": "https://issuer.example.com/authorize", + } + + @pytest.fixture + def token_response(self): + return {"access_token": "access-tok", "token_type": "Bearer", "expires_in": 3600} + + @pytest.mark.anyio + async def test_token_exchange_grant(self, config, mock_configuration, token_response): + with ( + patch.object(config, "configuration", new_callable=AsyncMock, return_value=mock_configuration), + patch.object(_OAuth2Client, "fetch_token", return_value=token_response) as mock_fetch, + patch.object(_OAuth2Client, "close") as mock_close, + ): + result = await config.token_exchange_grant(token="id-token-value", connector_id="ldap") + + assert result == token_response + call_kwargs = mock_fetch.call_args + assert call_kwargs.args[0] == "https://issuer.example.com/token" + assert call_kwargs.kwargs["grant_type"] == "urn:ietf:params:oauth:grant-type:token-exchange" + assert call_kwargs.kwargs["subject_token"] == "id-token-value" + assert call_kwargs.kwargs["connector_id"] == "ldap" + mock_close.assert_called_once() + + @pytest.mark.anyio + async def test_refresh_token_grant(self, config, mock_configuration, token_response): + with ( + patch.object(config, "configuration", new_callable=AsyncMock, return_value=mock_configuration), + patch.object(_OAuth2Client, "fetch_token", return_value=token_response) as mock_fetch, + patch.object(_OAuth2Client, "close") as mock_close, + ): + result = await config.refresh_token_grant(refresh_token="refresh-tok") + + assert result == token_response + call_kwargs = mock_fetch.call_args + assert call_kwargs.args[0] == "https://issuer.example.com/token" + assert call_kwargs.kwargs["grant_type"] == "refresh_token" + assert call_kwargs.kwargs["refresh_token"] == "refresh-tok" + mock_close.assert_called_once() + + @pytest.mark.anyio + async def test_password_grant(self, config, mock_configuration, token_response): + with ( + patch.object(config, "configuration", new_callable=AsyncMock, return_value=mock_configuration), + patch.object(_OAuth2Client, "fetch_token", return_value=token_response) as mock_fetch, + patch.object(_OAuth2Client, "close") as mock_close, + ): + result = await config.password_grant(username="testuser", password="testpass") + + assert result == token_response + call_kwargs = mock_fetch.call_args + assert call_kwargs.args[0] == "https://issuer.example.com/token" + assert call_kwargs.kwargs["grant_type"] == "password" + assert call_kwargs.kwargs["username"] == "testuser" + assert call_kwargs.kwargs["password"] == "testpass" + mock_close.assert_called_once() + + @pytest.mark.anyio + async def test_grant_closes_client_on_error(self, config, mock_configuration): + with ( + patch.object(config, "configuration", new_callable=AsyncMock, return_value=mock_configuration), + patch.object(_OAuth2Client, "fetch_token", side_effect=requests.HTTPError("500")), + patch.object(_OAuth2Client, "close") as mock_close, + ): + with pytest.raises(requests.HTTPError): + await config.password_grant(username="u", password="p") + + mock_close.assert_called_once() + + +class TestNoAuthlibDeprecationWarning: + def test_import_oidc_module_does_not_raise_deprecation_warning(self): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + import importlib + + import jumpstarter_cli_common.oidc + + importlib.reload(jumpstarter_cli_common.oidc) + deprecation_warnings = [ + w for w in caught if issubclass(w.category, DeprecationWarning) + ] + assert deprecation_warnings == [], ( + f"DeprecationWarning raised on import: {deprecation_warnings}" + ) diff --git a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py index c97e10df1..9223b7614 100644 --- a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py +++ b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py @@ -5,7 +5,7 @@ import time from dataclasses import dataclass from functools import wraps -from typing import ClassVar +from typing import Any, ClassVar from urllib.parse import parse_qs, urlencode, urlparse import aiohttp @@ -21,31 +21,41 @@ from jumpstarter.config.env import JMP_OIDC_CALLBACK_PORT -class _OAuth2Client: - """Lightweight OAuth2 client using requests.Session. +class OAuthError(Exception): + """Raised when an OAuth2 error response is received.""" + + +class OAuthStateMismatchError(OAuthError): + """Raised when the OAuth2 state parameter does not match the expected value.""" - Replaces authlib.integrations.requests_client.OAuth2Session to avoid - the AuthlibDeprecationWarning triggered by authlib's internal - authlib.jose imports (see https://github.com/jumpstarter-dev/jumpstarter/issues/627). - """ + +class _OAuth2Client: + """Minimal OAuth2 client wrapping requests.Session for authorization code, + password, token-exchange, and refresh-token grants.""" def __init__(self, client_id: str, scope: list[str], redirect_uri: str | None = None): self.client_id = client_id self.scope = scope self.redirect_uri = redirect_uri self._session = requests.Session() + self._expected_state: str | None = None @property - def verify(self): + def verify(self) -> bool | str: return self._session.verify @verify.setter - def verify(self, value): + def verify(self, value: bool | str) -> None: self._session.verify = value + def close(self) -> None: + """Close the underlying requests session.""" + self._session.close() + def create_authorization_url(self, url: str, **kwargs) -> tuple[str, str]: """Build an authorization URL with a generated state parameter.""" state = secrets.token_urlsafe(32) + self._expected_state = state params = { "response_type": "code", "client_id": self.client_id, @@ -58,15 +68,30 @@ def create_authorization_url(self, url: str, **kwargs) -> tuple[str, str]: separator = "&" if "?" in url else "?" return f"{url}{separator}{urlencode(params)}", state - def fetch_token(self, url: str, **kwargs) -> dict: + def fetch_token(self, url: str, **kwargs) -> dict[str, Any]: """Exchange credentials for an OAuth2 token via HTTP POST.""" - data = {"client_id": self.client_id} + data: dict[str, Any] = {"client_id": self.client_id} - # Handle authorization_response: extract the code from the callback URL authorization_response = kwargs.pop("authorization_response", None) if authorization_response: parsed = urlparse(authorization_response) qs = parse_qs(parsed.query) + + error = qs.get("error", [None])[0] + if error: + description = qs.get("error_description", [""])[0] + raise OAuthError( + f"OAuth2 authorization error: {error}" + + (f" - {description}" if description else "") + ) + + if self._expected_state is not None: + returned_state = qs.get("state", [None])[0] + if returned_state != self._expected_state: + raise OAuthStateMismatchError( + "OAuth2 state mismatch: possible CSRF attack" + ) + code = qs.get("code", [None])[0] if code: data["code"] = code @@ -74,20 +99,26 @@ def fetch_token(self, url: str, **kwargs) -> dict: data["redirect_uri"] = self.redirect_uri data.setdefault("grant_type", "authorization_code") - # Merge remaining kwargs into the POST body data.update(kwargs) - # Ensure scope is a space-separated string if "scope" not in data: data["scope"] = " ".join(self.scope) response = self._session.post( url, data=data, - headers={"Accept": "application/json"}, + headers={ + "Accept": "application/json", + "Content-Type": "application/x-www-form-urlencoded", + }, ) response.raise_for_status() - return response.json() + try: + return response.json() + except ValueError as exc: + raise OAuthError( + f"Token endpoint returned non-JSON response (HTTP {response.status_code})" + ) from exc def _get_ssl_context() -> ssl.SSLContext: @@ -154,43 +185,46 @@ def client(self, **kwargs): async def token_exchange_grant(self, token: str, **kwargs): config = await self.configuration() - client = self.client() - - return await run_sync( - lambda: client.fetch_token( - config["token_endpoint"], - grant_type="urn:ietf:params:oauth:grant-type:token-exchange", - requested_token_type="urn:ietf:params:oauth:token-type:access_token", - subject_token_type="urn:ietf:params:oauth:token-type:id_token", - subject_token=token, - **kwargs, + try: + return await run_sync( + lambda: client.fetch_token( + config["token_endpoint"], + grant_type="urn:ietf:params:oauth:grant-type:token-exchange", + requested_token_type="urn:ietf:params:oauth:token-type:access_token", + subject_token_type="urn:ietf:params:oauth:token-type:id_token", + subject_token=token, + **kwargs, + ) ) - ) + finally: + client.close() async def refresh_token_grant(self, refresh_token: str): config = await self.configuration() - client = self.client() - - return await run_sync( - lambda: client.fetch_token( - config["token_endpoint"], - grant_type="refresh_token", - refresh_token=refresh_token, + try: + return await run_sync( + lambda: client.fetch_token( + config["token_endpoint"], + grant_type="refresh_token", + refresh_token=refresh_token, + ) ) - ) + finally: + client.close() async def password_grant(self, username: str, password: str): config = await self.configuration() - client = self.client() - - return await run_sync( - lambda: client.fetch_token( - config["token_endpoint"], grant_type="password", username=username, password=password + try: + return await run_sync( + lambda: client.fetch_token( + config["token_endpoint"], grant_type="password", username=username, password=password + ) ) - ) + finally: + client.close() async def authorization_code_grant(self, callback_port: int | None = None, prompt: str | None = None): config = await self.configuration() @@ -244,9 +278,12 @@ async def callback(request): await site.stop() await runner.cleanup() - return await run_sync( - lambda: client.fetch_token(config["token_endpoint"], authorization_response=authorization_response) - ) + try: + return await run_sync( + lambda: client.fetch_token(config["token_endpoint"], authorization_response=authorization_response) + ) + finally: + client.close() def decode_jwt(token: str): From 373ec535a429d41d37edc4e8bb465c71c5964c48 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Thu, 28 May 2026 12:33:48 +0000 Subject: [PATCH 4/5] fix: restrict anyio tests to asyncio backend Add anyio_backend fixture to oauth2_client_test.py to match the project convention (see exceptions_test.py). Without this, pytest-anyio runs async tests with both asyncio and trio backends, but trio is not installed, causing ModuleNotFoundError failures in CI. Co-Authored-By: Claude Opus 4.6 --- .../jumpstarter_cli_common/oauth2_client_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oauth2_client_test.py b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oauth2_client_test.py index 058fe28f3..758b304d2 100644 --- a/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oauth2_client_test.py +++ b/python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oauth2_client_test.py @@ -13,6 +13,11 @@ ) +@pytest.fixture +def anyio_backend(): + return "asyncio" + + class TestOAuth2ClientInit: def test_init_sets_attributes(self): client = _OAuth2Client(client_id="my-client", scope=["openid", "profile"]) From 45f69d60d9b672a47239c2f8868c51e2679304c6 Mon Sep 17 00:00:00 2001 From: "ambient-code[bot]" <235912155+ambient-code[bot]@users.noreply.github.com> Date: Fri, 29 May 2026 16:34:17 +0000 Subject: [PATCH 5/5] chore: trigger fresh CI run The previous e2e-tests run failed with a transient infrastructure timeout unrelated to this PR's OAuth2 changes. Triggering a fresh CI run to verify all tests pass. Co-Authored-By: Claude Opus 4.6