feat(spp_oauth): prepare spp_oauth for stable status with RS256 bridge module#114
feat(spp_oauth): prepare spp_oauth for stable status with RS256 bridge module#114gonzalesedwin1123 wants to merge 4 commits into19.0from
Conversation
…rage Empty default config parameter values so get_private_key()/get_public_key() raise clear OpenSPPOAuthJWTException when keys are not configured, instead of failing with a cryptic PyJWT error on the placeholder strings. Also: - Export get_private_key/get_public_key from tools __init__ - Remove unused MOCK_PRIVATE_KEY constant and deprecated default_backend() - Move inline imports to top of test_rsa_encode_decode.py - Add test_res_config_settings.py covering settings model - Add test_exception_logs_error asserting _logger.error is called - Add QA testing guide (USAGE.md) with 8 test scenarios - Update DESCRIPTION.md with missing functions and cryptography dep
Bridge module that auto-installs when both spp_api_v2 and spp_oauth are present, enabling RS256 JWT authentication for the API alongside the existing HS256 flow. - Override FastAPI auth dependency to route by JWT header alg field - Add /oauth/token/rs256 endpoint for RS256 token generation - Explicit algorithm allowlist (RS256/HS256 only) to prevent confusion attacks - 22 tests covering RS256 auth, HS256 regression, and token generation
…indings Rename abbreviated field/param names to full forms per naming conventions: - oauth_priv_key → oauth_private_key - oauth_pub_key → oauth_public_key across models, config params, views, data, tests, and docs. spp_oauth changes: - Category: "OpenSPP" → "OpenSPP/Integration" - ACL: restrict to base.group_system with perm_create=1 - Remove ERROR logging from exception constructor (let callers decide) - Rename model class RegistryConfig → OAuthConfig - Add wrong-key verification test - Improve test assertions (assertIsNotNone) and remove numbered prefixes - Fix stale doc references to removed logging behavior spp_api_v2_oauth changes: - Remove empty ir.model.access.csv (no new models) - Add RS256 endpoint to API auth allowlist - Fix nosemgrep annotation for multiline sudo() - Remove Python <3.11 compat guard (Odoo 19 requires 3.11+) - Import constants from ..constants instead of redeclaring - Replace silent if-guards with skipTest() for endpoint tests - Safe int() cast for token_lifetime_hours config - asyncio.get_event_loop() → asyncio.run() - Manual key restore → addCleanup for test safety - SimpleNamespace for mock credentials
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the OpenSPP API's authentication capabilities by introducing support for RS256 JSON Web Tokens (JWTs) through a new bridge module. It also refines the existing OAuth module, improving its maintainability, security, and user experience. The changes aim to provide more robust and flexible authentication options, particularly beneficial for distributed and zero-trust architectures, while ensuring backward compatibility for existing HS256 clients. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements by adding RS256 JWT authentication support through a new spp_api_v2_oauth bridge module, while also improving the existing spp_oauth module. The changes are well-structured, with comprehensive test coverage for both new functionality and regression testing for existing HS256 authentication. Key improvements include stricter access controls on OAuth key configuration, clearer naming conventions, and better error handling. The documentation has also been thoroughly updated. My review focuses on a couple of opportunities to reduce code duplication to further enhance long-term maintainability. Overall, this is an excellent contribution that strengthens the platform's authentication capabilities.
I am having trouble creating individual review comments. Click here to see my feedback.
spp_api_v2_oauth/constants.py (7-8)
The constants JWT_AUDIENCE and JWT_ISSUER are also used in the spp_api_v2 module, where they are currently hardcoded. While defining them as constants here is an improvement, it introduces duplication between modules. To maintain a single source of truth, consider defining these constants in a shared location, perhaps within the spp_api_v2 module itself (since spp_api_v2_oauth depends on it), and importing them here. This would make future updates to these values safer and more maintainable.
spp_api_v2_oauth/routers/oauth_rs256.py (73-81)
The logic to retrieve and parse the token_lifetime_hours configuration parameter, including the try-except block and default value handling, is duplicated from the original HS256 token endpoint in spp_api_v2. To improve maintainability and avoid potential inconsistencies, this logic could be extracted into a shared utility function that both token endpoints can call.
spp_oauth/tools/oauth_exception.py (1-9)
Removing the automatic error logging from the exception's constructor is a good design choice. It correctly places the responsibility of logging on the caller, allowing for more context-specific log levels and messages. However, the exception message itself could be more informative for the developer who needs to debug it. Consider including the original error message when wrapping other exceptions.
class OpenSPPOAuthJWTException(Exception):
"""Raised when an OAuth JWT operation fails (missing keys, invalid tokens, etc.)."""
pass
spp_oauth/tools/rsa_encode_decode.py (48-49)
The variable name privkey is an abbreviation. For consistency with the other changes in this PR (e.g., oauth_private_key), it would be clearer to use the full name private_key.
private_key = get_private_key(env)
return jwt.encode(headers=header, payload=payload, key=private_key, algorithm=JWT_ALGORITHM)
spp_oauth/tools/rsa_encode_decode.py (61-63)
The variable name pubkey is an abbreviation. For consistency with other variable names and to improve clarity, it's better to use the full name public_key.
public_key = get_public_key(env)
try:
return jwt.decode(access_token, key=public_key, algorithms=[JWT_ALGORITHM])
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #114 +/- ##
==========================================
+ Coverage 70.76% 70.83% +0.06%
==========================================
Files 682 689 +7
Lines 37347 37469 +122
==========================================
+ Hits 26429 26541 +112
- Misses 10918 10928 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…l-disclosure warnings Semgrep's python-logger-credential-disclosure rule flags log strings containing "token" as potential secret leaks. Reword messages to avoid the keyword while preserving log clarity.
Response to Gemini Code ReviewThanks for the review. Here's the status of each finding: ✅ Already FixedFinding #4 ( ℹ️ No Action NeededFinding #3 (exception class 📋 Valid But Out of ScopeFinding #1 (duplicated Fixing this properly requires modifying These would be good follow-up items for a |
1 participant
Summary
spp_oauthspp_api_v2_oauthbridge module enabling RS256 JWT authentication for API V2Changes
spp_oauth
"OpenSPP"→"OpenSPP/Integration"per module-visibility principlesoauth_priv_key→oauth_private_key,oauth_pub_key→oauth_public_key) across models, config params, views, data, tests, and docsbase.group_usertobase.group_systemwithperm_create=1RegistryConfig→OAuthConfigfor clarityassertIsNotNone), remove numbered prefixesspp_api_v2_oauth (new module)
Bridge module that auto-installs when both
spp_api_v2andspp_oauthare present, enabling RS256 JWT authentication alongside existing HS256 support.get_authenticated_clientvia FastAPI dependency override; route by JWT headeralgfield (RS256/HS256 only, reject unknown)POST /oauth/token/rs256with same client auth flow, rate limiting, and payload structure as HS256auto_install: ["spp_api_v2", "spp_oauth"],application: False, categoryOpenSPP/IntegrationTest plan
./scripts/test_single_module.sh spp_oauth— 14/14 passing./scripts/test_single_module.sh spp_api_v2_oauth— 31/31 passingpre-commit run— all 37 hooks passing (ruff, semgrep, bandit, pylint, OpenSPP checks)/oauth/token/rs256endpoint returns valid RS256 tokens when keys are configured