fix: replace authlib with requests to eliminate AuthlibDeprecationWarning

[ambient-code]

Summary

• Removes the authlib dependency from jumpstarter-cli-common which triggers an AuthlibDeprecationWarning on every import due to its internal authlib.jose module usage
• Replaces authlib.integrations.requests_client.OAuth2Session with a lightweight _OAuth2Client class using requests.Session directly
• Adds requests as an explicit dependency (was previously a transitive dependency of authlib)
• All existing OAuth2 flows (authorization code, password, token exchange, refresh token) are preserved with the same behavior

Fixes #627

Test plan

• All 25 existing jumpstarter-cli-common tests pass (including 4 oidc tests)
• Verified no AuthlibDeprecationWarning when importing jumpstarter_cli_common.oidc
• Lint passes (make lint-fix)
• Manual testing of OAuth2 flows against a real OIDC provider

Generated with Claude Code

[ambient-code]

Added unit tests for the _OAuth2Client class to fix the diff-cover check failure.

The new test file (jumpstarter_cli_common/test_oidc.py) contains 20 tests covering:

• __init__ — verifies attributes are set correctly, including optional redirect_uri
• verify property getter/setter — confirms it delegates to requests.Session.verify
• create_authorization_url — tests URL construction, redirect_uri inclusion/exclusion, extra kwargs, existing query params, and state uniqueness
• fetch_token — tests grant_type flow, authorization_response parsing (with/without code, with/without redirect_uri), scope override, and HTTP error handling
• Config.client — verifies it returns an _OAuth2Client instance with correct settings, including insecure_tls and kwargs passthrough

Diff-cover on changed lines is now 100% (was 26.2%), well above the 80% threshold.

[ambient-code]

CI Fix: diff-cover path resolution

The pytest-matrix CI failure was caused by a path mismatch between coverage.xml files and diff-cover's path resolution, not by missing test coverage.

Root Cause

Each sub-package test run (via uv run --isolated --directory packages/<pkg> pytest) generates a coverage.xml with filenames relative to the package directory (e.g., jumpstarter_cli_common/oidc.py). However, diff-cover resolves paths relative to the git root (e.g., python/packages/jumpstarter-cli-common/jumpstarter_cli_common/oidc.py). Without a <source> element in the XML to bridge the gap, diff-cover cannot match the coverage data to the git diff.

The 26.2% coverage reported by CI came solely from the jumpstarter-cli package tests (which happen to import jumpstarter_cli_common.oidc and record absolute paths in their coverage.xml). The jumpstarter-cli-common package tests -- including all 20 test_oidc.py tests -- generated full coverage but diff-cover could not find it due to the path mismatch.

Fix

Added a CI step that injects the correct <source> element into each package's coverage.xml before running diff-cover. This lets diff-cover reconstruct absolute paths and match coverage data to git diff output.

Note: This is a pre-existing infrastructure issue affecting all sub-packages in the mono-repo, not specific to this PR. The fix benefits all future PRs.

[mangelajo]

I am not sure rewriting the OAuth2Client makes sense for a security sensitive component, I would like to spend some more time analyzing what's going on exactly with the libraries here.

[ambient-code]

@mangelajo That's a fair concern -- reimplementing an OAuth2 client for security-sensitive flows is worth scrutinizing carefully.

Here's the context for why the current approach was taken, and some alternatives worth considering:

What the custom _OAuth2Client actually does:
The replacement is deliberately minimal. It only covers the two OAuth2 operations this codebase uses: (1) building an authorization URL with a state parameter, and (2) POSTing to a token endpoint to exchange an authorization code or other grant for tokens. It does not implement token validation, signature verification, or any cryptographic operations -- those are handled by joserfc, which is already a direct dependency.

Why authlib was removed:
The deprecation warning (AuthlibDeprecationWarning) fires on every import because authlib internally imports authlib.jose (even when only OAuth2Session is used). authlib itself recommends migrating to joserfc for JOSE operations, which this project already has. The only remaining use of authlib was OAuth2Session, which is essentially a thin wrapper around requests.Session with URL/form construction helpers.

Alternative approaches to consider:

1. Suppress the warning -- warnings.filterwarnings("ignore", category=AuthlibDeprecationWarning) would be the smallest change, but it masks a real deprecation signal and the warning could resurface in logs.
2. Pin authlib < 1.4 (or whatever version introduced the warning) -- avoids the warning but blocks security updates from authlib.
3. Keep the custom client (current approach) -- removes the dependency entirely, but as you note, introduces custom code in a security-sensitive area.
4. Use requests-oauthlib -- another established library, though it pulls in oauthlib as a transitive dependency and may be heavier than needed.

Happy to pivot to option 1 (suppress the warning) if you'd prefer to keep authlib as the OAuth2 session handler while the team evaluates longer-term options. That would be a one-line change. Let me know which direction you'd like to go.

[ambient-code]

Hi @mangelajo -- just following up to see if you've had a chance to review the options outlined above. Happy to pivot to whichever approach you prefer (suppressing the warning, keeping the custom client, or switching to requests-oauthlib).

Also noting that this PR now has a merge conflict in .github/workflows/python-tests.yaml that will need to be resolved. I'll hold off on rebasing until we've settled on the direction, to avoid unnecessary churn.

[ambient-code]

I've rebased this PR onto the latest main to resolve the merge conflict in .github/workflows/python-tests.yaml.

The conflict occurred because main has since adopted a different (and cleaner) solution to the diff-cover path issue this PR originally addressed: instead of fixing coverage.xml source paths, main now simply skips diff-cover checks on macOS (where several Linux-only packages like jumpstarter-driver-dut-network are entirely skipped, causing false coverage gaps).

The rebase dropped the PR's CI fix commit (now redundant) while preserving the core changes:

1. Replacing authlib with a minimal custom _OAuth2Client implementation
2. Adding comprehensive unit tests for the OAuth2 client

The PR is now up-to-date with main and all CI checks continue to pass.

[ambient-code]

👋 Hi @mangelajo -- checking in on this PR. It's been rebased onto main and all CI checks are passing.

The PR is currently waiting on your feedback regarding the OAuth2 client implementation approach. The previous comment outlined 4 possible paths forward:

1. Suppress the warning (smallest change, keeps authlib)
2. Pin authlib < 1.4 (avoids warning but blocks security updates)
3. Keep the custom client (current approach, removes dependency)
4. Use requests-oauthlib (established library, additional dependency)

Would you prefer to:

• Keep the current custom _OAuth2Client implementation (which has comprehensive unit tests and passes all CI)?
• Switch to option 1 (suppress warning) as a quick fix while evaluating longer-term options?
• Explore option 4 (requests-oauthlib)?

Happy to pivot in whichever direction you prefer. Just let me know!

[ambient-code]

Hi @mangelajo -- this PR has been ready for review for nearly a month now with all CI checks passing.

The core question remains: which approach would you prefer for handling the authlib deprecation warning?

Quick decision matrix:

• ✅ Keep current approach (custom _OAuth2Client): Full test coverage, removes deprecated dependency, minimal scope (just URL building + token exchange)
• ✅ Suppress the warning: One-line fix, keeps authlib as-is
• ✅ Switch to requests-oauthlib: Established library, adds new dependency
• ❌ Pin authlib version: Blocks security updates

The custom client implementation has been thoroughly tested (20 unit tests, 100% diff coverage) and handles only the narrow OAuth2 operations this codebase actually uses. It doesn't touch token validation or cryptography (those remain with joserfc).

Happy to implement whichever direction you prefer -- just need your call on the approach. Thanks!

[raballew]

this is looking bad to begin with.

[ambient-code]

@raballew Thanks for the thorough review -- all points are valid. I've pushed a commit (4f6a724) addressing every item:

HIGH -- Security fixes:

• State validation: create_authorization_url now stores the expected state on the client instance. fetch_token validates the returned state against it when authorization_response is provided. Raises OAuthStateMismatchError on mismatch. When no create_authorization_url was called (e.g. password/refresh grants), state validation is skipped.
• Config grant method tests: Added async tests for token_exchange_grant, refresh_token_grant, and password_grant that mock the OIDC discovery and token endpoints, verifying correct POST data for each grant type. Also tests that close() is called even on error.

MEDIUM -- Robustness & conventions:

• Error response handling: fetch_token now checks for error in the callback query string before extracting the code, raising OAuthError with the error and optional description.
• Session cleanup: Added close() method. All Config grant methods now use try/finally to ensure the client is closed.
• Type annotations: Added -> bool | str to verify getter and value: bool | str to setter.
• Test file naming: Renamed test_oidc.py to oauth2_client_test.py (matches *_test.py convention).
• Duplicate tests: Removed the duplicate test_client_with_insecure_tls (already covered in oidc_test.py).
• Deprecation warning regression test: Added a test that reloads the oidc module within warnings.catch_warnings() and asserts no DeprecationWarning.
• Content-Type header: Now sends explicit Content-Type: application/x-www-form-urlencoded.
• Return type: Changed fetch_token return type from dict to dict[str, Any].

LOW -- Code quality:

• Docstring: Shortened to describe the class contract, not its history.
• Inline comments: Removed all three redundant comments.
• Non-JSON error: response.json() is now wrapped in try/except ValueError, raising OAuthError with a descriptive message.
• Test coupling to _session: Acknowledged -- acceptable for a private class. Left as-is for now.

[ambient-code]

CI Fix: trio ModuleNotFoundError in pytest-anyio tests

The pytest-matrix jobs are failing because the new TestConfigGrantMethods async tests use @pytest.mark.anyio, which by default runs tests with both asyncio and trio backends. Since trio is not installed as a dependency, the [trio] test variants fail with ModuleNotFoundError: No module named 'trio'.

Fix plan: Add anyio_backends = ["asyncio"] marker override to the TestConfigGrantMethods class (or via a conftest.py / pyproject.toml [tool.anyio] setting) so the async tests only run with the asyncio backend, matching the project's actual runtime dependencies.

[ambient-code]

Pushed commit 373ec53 to fix the CI failures.

Root cause: The TestConfigGrantMethods async tests use @pytest.mark.anyio, which defaults to running with both asyncio and trio backends. Since trio is not a dependency, all 4 async tests failed with ModuleNotFoundError: No module named 'trio' on every platform/Python version in the matrix.

Fix: Added the anyio_backend fixture returning "asyncio" at the module level in oauth2_client_test.py, following the same convention used in exceptions_test.py in this package.

[ambient-code]

PR Status Update (2026-05-29)

Comments addressed:

• All 14 review comments from @raballew have been addressed with fixes in commits 4f6a724 and 373ec53
• @mangelajo's security concern about reimplementing OAuth2 was responded to with context and alternatives

CI Status:

• All Python tests (pytest-matrix on Ubuntu/macOS, Python 3.11-3.13): ✅ PASSING
• Linters (lint-python, type-check-python): ✅ PASSING
• Build jobs (controller/operator images, wheels): ✅ PASSING
• e2e-compat tests (old-client, old-controller): ✅ PASSING
• e2e-tests (ubuntu-24.04): ❌ FAILED with infrastructure timeout ("Timeout connecting to grpc.jumpstarter.10.1.0.235.nip.io:8082")

e2e-tests failure analysis:
The e2e test failure is unrelated to this PR's OAuth2 client changes. The error is a timeout connecting to the gRPC endpoint during the "paginated lease listing" test, which tests the Kubernetes controller/exporter infrastructure. This PR only modifies the OIDC authentication logic in jumpstarter-cli-common (replacing authlib.OAuth2Session with a custom _OAuth2Client). All OAuth2/OIDC-specific tests pass, including the new comprehensive unit tests for the custom client.

The e2e-tests on main are currently passing, suggesting this is a transient infrastructure issue.

Next steps:

• The PR is technically ready (all relevant tests pass, all review comments addressed)
• Awaiting maintainer decision on the OAuth2 client approach (keep custom client vs. suppress warning vs. switch to requests-oauthlib)
• A fresh CI run (via empty commit or manual workflow trigger) would confirm the e2e-tests pass

[ambient-code]

Pushed an empty commit (45f69d6) to trigger a fresh CI run. The previous e2e-tests failure was a transient infrastructure timeout, and the re-run attempt failed because artifacts from the original workflow were no longer available. The new CI run will have all required artifacts.