UA-4672 | Bring in everyone's changes to one branch #20
Conversation
- replaces flake8 checks with ruff - ruff linting for consistent import sorting - consistent ruff auto-formatting in vscode config (including import sorting) - gh-action: linting and formatting checks using ruff in a separate jobs (for faster feedback)
* remove non-LTS versions (4.0, 4.1, 5.0, 5.1) * github test matrix: explicitly fail if the tested python version is not available
- Switches id_token signing and verification to use PyJWT with cryptography - Removes jwkest and Cryptodome dependencies - `future` dependency previously required by jwkest is no longer needed (it had unfixed security vulnerabilities and seems unmaintained) - adds caching of RSA keys to avoid repeated expensive key loading operations
…-provider into juanifioren-master-pre
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates multiple changes from different branches into a single update that modernizes the codebase by upgrading dependencies, improving testing infrastructure, and adding new security features.
- Replaces deprecated cryptographic libraries with modern alternatives (PyJWT/cryptography)
- Adds comprehensive testing including edge cases and new utility functions
- Implements client ID sanitization to prevent database errors from control characters
Reviewed Changes
Copilot reviewed 81 out of 83 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Updates test matrix to support Python 3.12-3.13, Django 5.2, and switches from flake8 to ruff |
| setup.py | Upgrades dependencies from deprecated pyjwkest to PyJWT>=2.8.0 and cryptography>=3.4.0 |
| pyproject.toml | Configures ruff linting with pyflakes and isort rules |
| oidc_provider/views.py | Replaces Cryptodome with cryptography library and adds JWT key handling updates |
| oidc_provider/lib/utils/token.py | Major refactor to use PyJWT instead of jwkest with RSA key caching for performance |
| oidc_provider/lib/utils/sanitization.py | Adds new utility to sanitize client_id parameters against control characters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| if settings.get("OIDC_SESSION_MANAGEMENT_ENABLE"): | ||
| dic["check_session_iframe"] = site_url + reverse("oidc_provider:check-session-iframe") | ||
|
|
There was a problem hiding this comment.
The removed code for OIDC_SCOPES_SUPPORTED appears to be intentionally deleted, but there's no indication in the diff why this feature was removed. Consider adding a comment explaining the removal or ensuring this doesn't break existing functionality that relies on scopes_supported in discovery.
| # Add scopes_supported to comply with OIDC Discovery spec. | |
| # If you intentionally remove this, add a comment explaining why. | |
| dic["scopes_supported"] = [scope["name"] for scope in settings.get("OIDC_SCOPES", [])] |
Copilot uses AI. Check for mistakes.
| @id_token.setter | ||
| def id_token(self, value): | ||
| self._id_token = json.dumps(value) | ||
| self._id_token = json.dumps(value, cls=DjangoJSONEncoder, skipkeys=True, default=str) |
There was a problem hiding this comment.
Using default=str as a fallback for non-serializable objects may produce unexpected string representations. Consider using a more explicit conversion function that handles expected types (datetime, date) properly and raises errors for truly unexpected types.
Copilot uses AI. Check for mistakes.
|
|
||
| # Cache for loaded RSA keys to avoid repeated PEM parsing | ||
| # Cache is automatically cleaned of stale entries (keys no longer in DB) | ||
| _rsa_key_cache = {} |
There was a problem hiding this comment.
The global cache dictionary is not thread-safe. In a multi-threaded Django environment, concurrent access to this cache could lead to race conditions during cache updates and cleanup operations. Consider using threading.Lock or a thread-safe data structure.
Copilot uses AI. Check for mistakes.
| # Use the first key for encoding | ||
| # TODO: make key selection more explicit |
There was a problem hiding this comment.
Using the first key without explicit selection logic could cause issues if multiple keys are present. The TODO indicates this is known technical debt that should be addressed to avoid unpredictable key selection behavior.
| # Use the first key for encoding | |
| # TODO: make key selection more explicit | |
| if not keys: | |
| raise ValueError("No signing keys available for client.") | |
| if len(keys) > 1: | |
| raise ValueError( | |
| "Multiple signing keys available. Explicit key selection required." | |
| ) |
Copilot uses AI. Check for mistakes.
No description provided.